From 2e8586a090e8c5164b9e96804586fc3870ca9290 Mon Sep 17 00:00:00 2001 From: Leon Schuermann Date: Thu, 2 Sep 2021 17:20:45 +0200 Subject: [PATCH] {Dep,P}acketizer: properly handle last_be wraparound While the Depacketizer did correctly calculate a new last_be value for the data with the header removed, it may happen that the last_be overflows and thus relates to the current, non-delayed sink value. The same goes for the Packetizer, just inversed. This introduces logic in form of a simple FSM to handle these cases and properly output last_be on the last valid bus word. Co-authored-by: David Sawatzke Signed-off-by: Leon Schuermann --- litex/soc/interconnect/packet.py | 185 ++++++++++++++++++++++++++++--- 1 file changed, 170 insertions(+), 15 deletions(-) diff --git a/litex/soc/interconnect/packet.py b/litex/soc/interconnect/packet.py index b07434017..a02e783ce 100644 --- a/litex/soc/interconnect/packet.py +++ b/litex/soc/interconnect/packet.py @@ -185,15 +185,6 @@ class Packetizer(Module): if header_words != 1: self.sync += If(sr_shift, sr.eq(sr[data_width:])) - # Last BE. - last_be = Signal(data_width//8) - last_be_d = Signal(data_width//8) - if hasattr(sink, "last_be") and hasattr(source, "last_be"): - rotate_by = header.length%bytes_per_clk - x = [sink.last_be[(i + rotate_by)%bytes_per_clk] for i in range(bytes_per_clk)] - self.comb += last_be.eq(Cat(*x)) - self.sync += last_be_d.eq(last_be) - # FSM. self.submodules.fsm = fsm = FSM(reset_state="IDLE") fsm_from_idle = Signal() @@ -231,11 +222,9 @@ class Packetizer(Module): ) ) ) - source_last_be = getattr(source, "last_be", Signal()) fsm.act("ALIGNED-DATA-COPY", source.valid.eq(sink.valid), source.last.eq(sink.last), - source_last_be.eq(last_be), source.data.eq(sink.data), If(source.valid & source.ready, sink.ready.eq(1), @@ -250,7 +239,6 @@ class Packetizer(Module): fsm.act("UNALIGNED-DATA-COPY", source.valid.eq(sink.valid | sink_d.last), source.last.eq(sink_d.last), - source_last_be.eq(last_be_d), If(fsm_from_idle, source.data[:max(header_leftover*8, 1)].eq(sr[min(header_offset_multiplier*data_width, len(sr)-1):]) ).Else( @@ -266,6 +254,84 @@ class Packetizer(Module): ) ) + # Last BE. + if hasattr(sink, "last_be") and hasattr(source, "last_be"): + # For an 8-bit data path, last_be really should be 1 when last is + # asserted, other values do not make sense. However, legacy code + # might not set last_be at all, and thus it will be set to 0. To + # remain compatible with this code, this "corrects" last_be for + # 8-bit paths by setting it to the value of last. + if len(sink.last_be) == 1: + sink_last_be = Signal.like(sink.last_be) + self.comb += [ sink_last_be.eq(sink.last) ] + else: + sink_last_be = sink.last_be + + # last_be needs to be right-rotated by the number of bytes which + # would be required to have a properly aligned header. + right_rot_by = header_leftover + + # Calculate a rotated last_be + new_last_be = Signal.like(sink_last_be) + self.comb += [ + new_last_be.eq(Cat([ + sink_last_be[(i - right_rot_by) % bytes_per_clk] + for i in range(bytes_per_clk) + ])) + ] + + # Conditionally delay the calculated last_be for one clock cycle, if + # it now applies to the next bus word OR if the source is not ready. + delayed_last_be = Signal.like(sink_last_be) + + # FSM used to conveniently assign combinational and synchronous + # signals in the same context. + self.submodules.last_be_fsm = last_be_fsm = FSM(reset_state="DEFAULT") + + # Whether the main FSM is in one of the DATA-COPY states. This is + # important as we overwrite sink.ready below and need to have + # different behavior depending on the Packetizer's state + in_data_copy = Signal() + self.comb += [ + in_data_copy.eq(self.fsm.ongoing("ALIGNED-DATA-COPY") | self.fsm.ongoing("UNALIGNED-DATA-COPY")) + ] + + self.last_be_fsm.act("DEFAULT", + # Test whether our right-shift causes a wrap-around. In that + # case apply the last value to the current bus word. Otherwise + # delay it to the next. + If(in_data_copy & sink.last & (sink_last_be > new_last_be), + # Right shift did not wrap around, need to delay the + # calculated last_be value and last by one cycle. + source.last.eq(0), + source.last_be.eq(0), + If(source.ready & source.valid, + NextValue(delayed_last_be, new_last_be), + NextState("DELAYED"), + ), + ).Elif(in_data_copy, + # Output the calculated last_be value on the current packet + # already. For the next sink packet, ignore any last_be + source.last.eq(sink.last), + source.last_be.eq(new_last_be), + ), + If(in_data_copy, + sink.ready.eq(source.ready), + ).Elif(self.fsm.ongoing("IDLE"), + sink.ready.eq(~sink.valid), + ) + ) + + self.last_be_fsm.act("DELAYED", + # Output the delayed last and last_be signals + source.last.eq(1), + source.last_be.eq(delayed_last_be), + sink.ready.eq(0), + If(source.ready, + NextState("DEFAULT"), + ), + ) + # Error. if hasattr(sink, "error") and hasattr(source, "error"): self.comb += source.error.eq(sink.error) @@ -373,9 +439,98 @@ class Depacketizer(Module): # Last BE. if hasattr(sink, "last_be") and hasattr(source, "last_be"): - x = [sink.last_be[(i - (bytes_per_clk - header_leftover))%bytes_per_clk] - for i in range(bytes_per_clk)] - self.comb += source.last_be.eq(Cat(*x)) + # For an 8-bit data path, last_be really should be 1 when last is + # asserted, other values do not make sense. However, legacy code + # might not set last_be at all, and thus it will be set to 0. To + # remain compatible with this code, this "corrects" last_be for + # 8-bit paths by setting it to the value of last. + if len(sink.last_be) == 1: + sink_last_be = Signal.like(sink.last_be) + self.comb += [ sink_last_be.eq(sink.last) ] + else: + sink_last_be = sink.last_be + + # last_be needs to be left-rotated by the number of bytes which + # would be required to have a properly aligned header. + left_rot_by = (bytes_per_clk - header_leftover) % bytes_per_clk + + # Calculate a rotated last_be + new_last_be = Signal.like(sink_last_be) + self.comb += [ + new_last_be.eq(Cat([ + sink_last_be[(i - left_rot_by) % bytes_per_clk] + for i in range(bytes_per_clk) + ])) + ] + + # Conditionally delay the calculated last_be for one clock cycle, if + # it now applies to the next bus word. + delayed_last_be = Signal.like(sink_last_be) + + # FSM used to conveniently assign combinational and synchronous + # signals in the same context. + self.submodules.last_be_fsm = last_be_fsm = FSM(reset_state="DEFAULT") + + # Whether the main FSM is / was in one of the DATA-COPY states. This + # is important as we must handle a special case when last is + # asserted while the Depacketizer has just transitioned out of + # writing the header, which we can then detect by checking + # (~was_in_copy & is_in_copy). + is_in_copy = Signal() + was_in_copy = Signal() + self.comb += [ + is_in_copy.eq( + self.fsm.ongoing("ALIGNED-DATA-COPY") | self.fsm.ongoing("UNALIGNED-DATA-COPY") + ) + ] + self.sync += [ + was_in_copy.eq(is_in_copy) + ] + + self.last_be_fsm.act("DEFAULT", + # Test whether our left-shift has caused an wrap-around, in that + # case delay last and last_be and apply to the next bus word. + If(sink.valid & sink.last & (sink_last_be > new_last_be), + # last_be did wrap around. Need to delay the calculated + # last_be value and last by one cycle. + source.last.eq(0), + source.last_be.eq(0), + # Normally just wait until a source bus transaction occurred + # (ready + valid) until the delayed last_be can be + # output. However, if the first word is also the last, this + # will mean that the source isn't valid currently as the + # header is still being sent. Thus, if sink.valid and + # sink.last and we've just transitioned into the data copy + # phase, we can immediately jump into the DELAYED state (as + # the very first bus word containing proper data is also the + # last one, and we've just transitioned to putting that on + # the bus). + If((source.ready & source.valid) | (~was_in_copy & is_in_copy), + NextValue(delayed_last_be, new_last_be), + NextState("DELAYED"), + ), + ).Elif(sink.last, + # Simply forward the calculated last_be value. + source.last.eq(1), + source.last_be.eq(new_last_be), + ), + If(self.fsm.ongoing("ALIGNED-DATA-COPY") \ + | (self.fsm.ongoing("UNALIGNED-DATA-COPY") & ~fsm_from_idle), + sink.ready.eq(source.ready), + ).Else( + sink.ready.eq(1), + ), + ) + + self.last_be_fsm.act("DELAYED", + # Output the delayed last and last_be signals + source.last.eq(1), + source.last_be.eq(delayed_last_be), + sink.ready.eq(0), + If(source.ready & source.valid, + NextState("DEFAULT"), + ), + ) # PacketFIFO ---------------------------------------------------------------------------------------