Merge pull request #385 from enjoy-digital/fix-upconverter-subword-ordering

frontend: preserve upconverter subword ordering

Author: enjoy-digital

litedram/frontend/adapter.py

@@ -150,16 +150,24 @@ def __init__(self, port_from, port_to, reverse=False):
 
         # # #
 
-        ratio = port_to.data_width//port_from.data_width
-        mode  = port_from.mode
+        ratio      = port_to.data_width//port_from.data_width
+        mode       = port_from.mode
+        chunk_bits = log2_int(ratio)
 
         # Command ----------------------------------------------------------------------------------
 
-        # Defines cmd type and the chunks that have been requested for the current port_to command.
-        sel              = Signal(ratio)
+        # Store the subword request order for the current port_to command.
+        # This preserves command order for reads and maps write data back to address lanes.
+        cmd_count        = Signal(max=ratio + 1)
+        cmd_order        = Signal(ratio*chunk_bits)
+        cmd_chunks       = Signal(ratio)
         cmd_sel          = Signal(ratio)
         cmd_selected     = Signal()
-        cmd_buffer       = stream.SyncFIFO([("sel", ratio), ("we", 1)], 0)
+        cmd_buffer       = stream.SyncFIFO([
+            ("we",    1),
+            ("count", len(cmd_count)),
+            ("order", len(cmd_order)),
+        ], 0)
         self.submodules += cmd_buffer
         # Store last received command.
         cmd_addr         = Signal.like(port_from.cmd.addr)
@@ -168,7 +176,7 @@ def __init__(self, port_from, port_to, reverse=False):
         # Indicates that we need to proceed to the next port_to command.
         next_cmd         = Signal()
         addr_changed     = Signal()
-        # Signals that indicate that write/read convertion has finished.
+        # Signals that indicate that write/read conversion has finished.
         wdata_finished   = Signal()
         rdata_finished   = Signal()
         # Used to prevent reading old memory value if previous command has written the same address.
@@ -188,7 +196,9 @@ def __init__(self, port_from, port_to, reverse=False):
                 NextValue(cmd_addr, port_from.cmd.addr),
                 NextValue(cmd_we, port_from.cmd.we),
                 NextValue(cmd_last, port_from.cmd.last),
-                NextValue(sel, cmd_sel),
+                NextValue(cmd_count, 1),
+                NextValue(cmd_order[:chunk_bits], port_from.cmd.addr[:chunk_bits]),
+                NextValue(cmd_chunks, cmd_sel),
                 If(port_from.cmd.we,
                     NextState("FILL"),
                 ).Else(
@@ -199,7 +209,7 @@ def __init__(self, port_from, port_to, reverse=False):
         fsm.act("CMD",
             port_to.cmd.valid.eq(1),
             port_to.cmd.we.eq(cmd_we),
-            port_to.cmd.addr.eq(cmd_addr[log2_int(ratio):]),
+            port_to.cmd.addr.eq(cmd_addr[chunk_bits:]),
             If(port_to.cmd.ready,
                 If(cmd_we,
                     NextState("NEW")
@@ -208,21 +218,30 @@ def __init__(self, port_from, port_to, reverse=False):
                 )
             )
         )
+        cmd_order_cases = {}
+        for i in range(ratio):
+            cmd_order_cases[i] = NextValue(
+                cmd_order[i*chunk_bits:(i + 1)*chunk_bits],
+                port_from.cmd.addr[:chunk_bits])
+
         fsm.act("FILL",
             If(next_cmd,
                 NextState("COMMIT")
-            ).Else(  # Acknowledge incomming commands, while filling `sel`.
+            ).Else(  # Acknowledge incoming commands, while filling the request order.
                 port_from.cmd.ready.eq(port_from.cmd.valid),
                 NextValue(cmd_last, port_from.cmd.last),
                 If(port_from.cmd.valid,
-                    NextValue(sel, sel | cmd_sel)
+                    NextValue(cmd_count, cmd_count + 1),
+                    Case(cmd_count, cmd_order_cases),
+                    NextValue(cmd_chunks, cmd_chunks | cmd_sel)
                 )
             )
         )
         fsm.act("COMMIT",
             cmd_buffer.sink.valid.eq(1),
-            cmd_buffer.sink.sel.eq(sel),
             cmd_buffer.sink.we.eq(cmd_we),
+            cmd_buffer.sink.count.eq(cmd_count),
+            cmd_buffer.sink.order.eq(cmd_order),
             If(cmd_buffer.sink.ready,
                 If(cmd_we,
                     NextState("CMD")
@@ -234,9 +253,9 @@ def __init__(self, port_from, port_to, reverse=False):
 
         self.comb += [
             cmd_sel.eq(1 << port_from.cmd.addr[:log2_int(ratio)]),
-            cmd_selected.eq((sel & cmd_sel) != 0),
+            cmd_selected.eq((cmd_chunks & cmd_sel) != 0),
             cmd_buffer.source.ready.eq(wdata_finished | rdata_finished),
-            addr_changed.eq(cmd_addr[log2_int(ratio):] != port_from.cmd.addr[log2_int(ratio):]),
+            addr_changed.eq(cmd_addr[chunk_bits:] != port_from.cmd.addr[chunk_bits:]),
             # 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),
             # Go to the next command if one of the following happens:
@@ -248,7 +267,7 @@ def __init__(self, port_from, port_to, reverse=False):
             #  - master requests a flush (even after the command has been sent).
             next_cmd.eq(addr_changed | (cmd_we != port_from.cmd.we)
                         | (port_from.cmd.valid & cmd_selected)
-                        | (sel == 2**ratio - 1) | cmd_last | port_from.flush),
+                        | (cmd_count == ratio) | cmd_last | port_from.flush),
         ]
 
         self.sync += [
@@ -267,102 +286,103 @@ def __init__(self, port_from, port_to, reverse=False):
         # Read Datapath ----------------------------------------------------------------------------
 
         if mode in ["read", "both"]:
-            # Queue received data not to loose it when it comes too fast.
+            # Queue received data not to lose it when it comes too fast.
             rdata_fifo = stream.SyncFIFO(port_to.rdata.description, ratio - 1)
-            rdata_converter = stream.StrideConverter(
-                description_from = port_to.rdata.description,
-                description_to   = port_from.rdata.description,
-                reverse          = reverse)
-            self.submodules +=  rdata_fifo, rdata_converter
-
-            # Shift register with a bitmask of current chunk.
-            rdata_chunk       = Signal(ratio, reset=1)
-            rdata_chunk_valid = Signal()
-            self.sync += \
-                If(rdata_converter.source.valid &
-                   rdata_converter.source.ready,
-                    rdata_chunk.eq(Cat(rdata_chunk[ratio-1], rdata_chunk[:ratio-1]))
-                )
+            self.submodules += rdata_fifo
+
+            rdata_count = Signal(max=ratio)
+            rdata_chunk = Signal(chunk_bits)
+            rdata_valid = Signal()
+
+            rdata_order_cases = {}
+            rdata_mux_cases   = {}
+            for i in range(ratio):
+                n = ratio - 1 - i if reverse else i
+                rdata_order_cases[i] = rdata_chunk.eq(
+                    cmd_buffer.source.order[i*chunk_bits:(i + 1)*chunk_bits])
+                rdata_mux_cases[i] = port_from.rdata.data.eq(
+                    rdata_fifo.source.data[
+                        n*port_from.data_width:(n + 1)*port_from.data_width])
 
             self.comb += [
-                # port_to -> rdata_fifo -> rdata_converter -> port_from
+                # port_to -> rdata_fifo -> order mux -> port_from
                 port_to.rdata.connect(rdata_fifo.sink),
-                rdata_fifo.source.connect(rdata_converter.sink),
-                rdata_chunk_valid.eq((cmd_buffer.source.sel & rdata_chunk) != 0),
-                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.
-                    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)
-                    ).Else(  # If this chunk was not requested in `sel`, ignore it.
-                        rdata_converter.source.ready.eq(1)
-                    ),
-                    rdata_finished.eq(rdata_converter.source.valid & rdata_converter.source.ready
-                                      & rdata_chunk[ratio - 1])
-                ),
+                Case(rdata_count, rdata_order_cases),
+                Case(rdata_chunk, rdata_mux_cases),
+                rdata_valid.eq(cmd_buffer.source.valid & ~cmd_buffer.source.we & rdata_fifo.source.valid),
+                port_from.rdata.valid.eq(rdata_valid),
+                rdata_fifo.source.ready.eq(rdata_finished),
+                rdata_finished.eq(rdata_valid & port_from.rdata.ready
+                                  & (rdata_count == (cmd_buffer.source.count - 1))),
+            ]
+            self.sync += [
+                If(rdata_finished,
+                    rdata_count.eq(0)
+                ).Elif(rdata_valid & port_from.rdata.ready,
+                    rdata_count.eq(rdata_count + 1)
+                )
             ]
 
         # Write Datapath ---------------------------------------------------------------------------
 
         if mode in ["write", "both"]:
-            # Queue write data not to miss it when the lower chunks haven't been reqested.
+            # Queue write data not to miss it when the lower chunks haven't been requested.
             wdata_fifo    = stream.SyncFIFO(port_from.wdata.description, ratio - 1)
             wdata_buffer  = stream.SyncFIFO(port_to.wdata.description, 1)
-            wdata_converter = stream.StrideConverter(
-                description_from = port_from.wdata.description,
-                description_to   = port_to.wdata.description,
-                reverse          = reverse)
-            self.submodules += wdata_converter, wdata_fifo, wdata_buffer
-
-            # Shift register with a bitmask of current chunk.
-            wdata_chunk       = Signal(ratio, reset=1)
-            wdata_chunk_valid = Signal()
-            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` bits to match the width of port_to.wdata.we.
-            wdata_sel = Signal.like(port_to.wdata.we)
-            if reverse:
-                wdata_sel_parts = [
-                    Replicate(cmd_buffer.source.sel[i], port_to.wdata.we.nbits // sel.nbits)
-                    for i in reversed(range(ratio))
-                ]
-            else:
-                wdata_sel_parts = [
-                    Replicate(cmd_buffer.source.sel[i], port_to.wdata.we.nbits // sel.nbits)
-                    for i in range(ratio)
+            self.submodules += wdata_fifo, wdata_buffer
+
+            wdata_count   = Signal(max=ratio)
+            wdata_chunk   = Signal(chunk_bits)
+            wdata_data    = Signal.like(port_to.wdata.data)
+            wdata_we      = Signal.like(port_to.wdata.we)
+            wdata_pending = Signal()
+            wdata_accept  = Signal()
+            wdata_last    = Signal()
+
+            wdata_order_cases = {}
+            wdata_store_cases = {}
+            for i in range(ratio):
+                n = ratio - 1 - i if reverse else i
+                wdata_order_cases[i] = wdata_chunk.eq(
+                    cmd_buffer.source.order[i*chunk_bits:(i + 1)*chunk_bits])
+                wdata_store_cases[i] = [
+                    wdata_data[
+                        n*port_from.data_width:(n + 1)*port_from.data_width
+                    ].eq(wdata_fifo.source.data),
+                    wdata_we[
+                        n*port_from.wdata.we.nbits:(n + 1)*port_from.wdata.we.nbits
+                    ].eq(wdata_fifo.source.we),
                 ]
 
-            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_fifo -> ordered wide buffer -> port_to
                 port_from.wdata.connect(wdata_fifo.sink),
                 wdata_buffer.source.connect(port_to.wdata),
-                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(wdata_converter.sink.ready),
-                    ).Else(  # If chunk is not valid, send any data and do not advance fifo.
-                        wdata_converter.sink.valid.eq(1),
-                    ),
-                ),
-                wdata_buffer.sink.valid.eq(wdata_converter.source.valid),
-                wdata_buffer.sink.data.eq(wdata_converter.source.data),
-                wdata_buffer.sink.we.eq(wdata_converter.source.we & wdata_sel),
-                wdata_converter.source.ready.eq(wdata_buffer.sink.ready),
-                wdata_finished.eq(wdata_converter.sink.valid & wdata_converter.sink.ready
-                                  & wdata_chunk[ratio-1]),
+                wdata_buffer.sink.valid.eq(wdata_pending),
+                wdata_buffer.sink.data.eq(wdata_data),
+                wdata_buffer.sink.we.eq(wdata_we),
+                Case(wdata_count, wdata_order_cases),
+                wdata_fifo.source.ready.eq(cmd_buffer.source.valid & cmd_buffer.source.we
+                                           & ~wdata_pending),
+                wdata_accept.eq(wdata_fifo.source.valid & wdata_fifo.source.ready),
+                wdata_last.eq(wdata_count == (cmd_buffer.source.count - 1)),
+                wdata_finished.eq(wdata_buffer.sink.valid & wdata_buffer.sink.ready),
+            ]
+
+            self.sync += [
+                If(wdata_finished,
+                    wdata_count.eq(0),
+                    wdata_data.eq(0),
+                    wdata_we.eq(0),
+                    wdata_pending.eq(0),
+                ).Elif(wdata_accept,
+                    Case(wdata_chunk, wdata_store_cases),
+                    If(wdata_last,
+                        wdata_pending.eq(1)
+                    ).Else(
+                        wdata_count.eq(wdata_count + 1)
+                    )
+                )
             ]
 
 # LiteDRAMNativePortConverter ----------------------------------------------------------------------

test/test_adapter.py

@@ -364,6 +364,66 @@ def main_generator(dut):
                 self.converter_readback_test(dut, pattern=[], mem_expected=mem_expected,
                                              main_generator=main_generator)
 
+    def test_up_converter_write_ordering(self):
+        # Verify that write data is placed according to subword addresses, not command order.
+        pattern = [
+            (0x03, 0x00),
+            (0x02, 0x11),
+            (0x01, 0x22),
+            (0x00, 0x33),
+            (0x12, 0x44),
+            (0x11, 0x55),
+            (0x13, 0x66),
+            (0x10, 0x77),
+        ]
+        mem_expected = [
+            # data, address
+            0x00112233,  # 0x00
+            0x00000000,  # 0x04
+            0x00000000,  # 0x08
+            0x00000000,  # 0x0c
+            0x66445577,  # 0x10
+            0x00000000,  # 0x14
+            0x00000000,  # 0x18
+            0x00000000,  # 0x1c
+        ]
+
+        for separate_rw in [True, False]:
+            with self.subTest(separate_rw=separate_rw):
+                dut = ConverterDUT(user_data_width=8, native_data_width=32,
+                                   mem_depth=len(mem_expected), separate_rw=separate_rw)
+                self.converter_readback_test(dut, pattern=pattern, mem_expected=mem_expected)
+
+    def test_up_converter_read_ordering(self):
+        # Verify that read data is returned in command order, not ascending subword order.
+        pattern = [
+            (0x03, 0x33),
+            (0x02, 0x22),
+            (0x01, 0x11),
+            (0x00, 0x00),
+        ]
+        mem_expected = [
+            # data, address
+            0x33221100,  # 0x00
+            0x00000000,  # 0x04
+            0x00000000,  # 0x08
+            0x00000000,  # 0x0c
+        ]
+
+        def main_generator(dut):
+            for adr, _ in pattern[:-1]:
+                yield from dut.read(adr, wait_data=False)
+            yield from dut.read(pattern[-1][0], wait_data=False, last=1)
+            yield from dut.read_driver.wait_all()
+
+        for separate_rw in [True, False]:
+            with self.subTest(separate_rw=separate_rw):
+                dut = ConverterDUT(user_data_width=8, native_data_width=32,
+                                   mem_depth=len(mem_expected), separate_rw=separate_rw)
+                dut.memory.mem = mem_expected.copy()
+                self.converter_readback_test(dut, pattern=pattern, mem_expected=mem_expected,
+                                             main_generator=main_generator)
+
     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,