Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for optional axis_tstrb #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for optional axis_tstrb #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ To receive data with an `AxiStreamSink` or `AxiStreamMonitor`, call `recv()`/`re
* `tready`: indicates sink is ready for data; optional, assumed `1` when absent
* `tlast`: marks the last cycle of a frame; optional, assumed `1` when absent
* `tkeep`: qualifies data byte, data bus width must be evenly divisible by `tkeep` signal width; optional, assumed `1` when absent
* `tstrb`: qualifies data byte, data bus width must be evenly divisible by `tstrb` signal width; optional, assumed equal to `tkeep` when absent
* `tid`: ID signal, can be used for routing; optional, assumed `0` when absent
* `tdest`: destination signal, can be used for routing; optional, assumed `0` when absent
* `tuser`: additional user data; optional, assumed `0` when absent
Expand All @@ -266,7 +267,7 @@ To receive data with an `AxiStreamSink` or `AxiStreamMonitor`, call `recv()`/`re
* _byte_size_: byte size (optional)
* _byte_lanes_: byte lane count (optional)

Note: _byte_size_, _byte_lanes_, `len(tdata)`, and `len(tkeep)` are all related, in that _byte_lanes_ is set from `tkeep` if it is connected, and `byte_size*byte_lanes == len(tdata)`. So, if `tkeep` is connected, both _byte_size_ and _byte_lanes_ will be computed internally and cannot be overridden. If `tkeep` is not connected, then either _byte_size_ or _byte_lanes_ can be specified, and the other will be computed such that `byte_size*byte_lanes == len(tdata)`.
Note: _byte_size_, _byte_lanes_, `len(tdata)`, `len(tkeep)`, and `len(tstrb)` are all related, in that _byte_lanes_ is set from `tkeep` or `tstrb` if either is connected, and `byte_size*byte_lanes == len(tdata)`. So, if either `tkeep` or `tstrb` is connected, both _byte_size_ and _byte_lanes_ will be computed internally and cannot be overridden. If `tkeep` and `tstrb` is not connected, then either _byte_size_ or _byte_lanes_ can be specified, and the other will be computed such that `byte_size*byte_lanes == len(tdata)`.

#### Attributes:

Expand Down Expand Up @@ -302,12 +303,13 @@ The `AxiStreamBus` object is a container for the interface signals. Currently,

#### `AxiStreamFrame` object

The `AxiStreamFrame` object is a container for a frame to be transferred via AXI stream. The `tdata` field contains the packet data in the form of a list of bytes, which is either a `bytearray` if the byte size is 8 bits or a `list` of `int`s otherwise. `tkeep`, `tid`, `tdest`, and `tuser` can either be `None`, an `int`, or a `list` of `int`s.
The `AxiStreamFrame` object is a container for a frame to be transferred via AXI stream. The `tdata` field contains the packet data in the form of a list of bytes, which is either a `bytearray` if the byte size is 8 bits or a `list` of `int`s otherwise. `tkeep`, `tstrb`, `tid`, `tdest`, and `tuser` can either be `None`, an `int`, or a `list` of `int`s.

Attributes:

* `tdata`: bytes, bytearray, or list
* `tkeep`: tkeep field, optional; list, each entry qualifies the corresponding entry in `tdata`. Can be used to insert gaps on the source side.
* `tstrb`: tstrb field, optional; list, each entry qualifies the corresponding entry in `tdata`. Used to signal padding bytes.
* `tid`: tid field, optional; int or list with one entry per `tdata`, last value used per cycle when sending.
* `tdest`: tdest field, optional; int or list with one entry per `tdata`, last value used per cycle when sending.
* `tuser`: tuser field, optional; int or list with one entry per `tdata`, last value used per cycle when sending.
Expand All @@ -317,8 +319,8 @@ Attributes:

Methods:

* `normalize()`: pack `tkeep`, `tid`, `tdest`, and `tuser` to the same length as `tdata`, replicating last element if necessary, initialize `tkeep` to list of `1` and `tid`, `tdest`, and `tuser` to list of `0` if not specified.
* `compact()`: remove `tdata`, `tid`, `tdest`, and `tuser` values based on `tkeep`, remove `tkeep`, compact `tid`, `tdest`, and `tuser` to an int if all values are identical.
* `normalize()`: pack `tkeep`, `tstrb`, `tid`, `tdest`, and `tuser` to the same length as `tdata`, replicating last element if necessary, initialize `tkeep` to list of `1`, `tstrb` == `tkeep`, and `tid`, `tdest`, and `tuser` to list of `0` if not specified.
* `compact()`: remove `tdata`, `tstrb`, `tid`, `tdest`, and `tuser` values based on `tkeep`, remove `tkeep`, compact `tid`, `tdest`, and `tuser` to an int if all values are identical.

### Address space abstraction

Expand Down
52 changes: 45 additions & 7 deletions cocotbext/axi/axis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,10 @@


class AxiStreamFrame:
def __init__(self, tdata=b'', tkeep=None, tid=None, tdest=None, tuser=None, tx_complete=None):
def __init__(self, tdata=b'', tkeep=None, tstrb=None, tid=None, tdest=None, tuser=None, tx_complete=None):
self.tdata = bytearray()
self.tkeep = None
self.tstrb = None
self.tid = None
self.tdest = None
self.tuser = None
Expand All @@ -52,6 +53,8 @@ def __init__(self, tdata=b'', tkeep=None, tid=None, tdest=None, tuser=None, tx_c
self.tdata = list(tdata.tdata)
if tdata.tkeep is not None:
self.tkeep = list(tdata.tkeep)
if tdata.tstrb is not None:
self.tstrb = list(tdata.tstrb)
if tdata.tid is not None:
if type(tdata.tid) in (int, bool):
self.tid = tdata.tid
Expand All @@ -73,12 +76,14 @@ def __init__(self, tdata=b'', tkeep=None, tid=None, tdest=None, tuser=None, tx_c
elif type(tdata) in (bytes, bytearray):
self.tdata = bytearray(tdata)
self.tkeep = tkeep
self.tstrb = tstrb
self.tid = tid
self.tdest = tdest
self.tuser = tuser
else:
self.tdata = list(tdata)
self.tkeep = tkeep
self.tstrb = tstrb
self.tid = tid
self.tdest = tdest
self.tuser = tuser
Expand All @@ -95,6 +100,11 @@ def normalize(self):
else:
self.tkeep = [1]*n

if self.tstrb is not None:
self.tstrb = self.tstrb[:n] + [self.tstrb[-1]]*(n-len(self.tstrb))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The open question is if tstrb should be checked here, to validate the validity of the tkeep, tstrb combination.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely should check for protocol violations when possible. The question is what to do: I'm thinking logging a warning or error would be appropriate, but an exception or assert I suppose could also be reasonable. Perhaps the best solution would be to have an option for enabling protocol assertions somewhere, which would generate exceptions instead of log messages.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, I suppose what we could do is roll with the log messages for now and then think about some config options for this sort of thing separately.

else:
self.tstrb = self.tkeep
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is deliberate. If tstrb is not used, it should match tkeep. This implies 2 possible states:

  • keep byte + byte is data
  • may discard byte

It also means the invalid state of tkeep low + tstrb high is avoided.


if self.tid is not None:
if type(self.tid) in (int, bool):
self.tid = [self.tid]*n
Expand Down Expand Up @@ -128,6 +138,8 @@ def compact(self):
del self.tdata[k]
if k < len(self.tkeep):
del self.tkeep[k]
if k < len(self.tstrb):
del self.tstrb[k]
if k < len(self.tid):
del self.tid[k]
if k < len(self.tdest):
Expand Down Expand Up @@ -172,6 +184,10 @@ def __eq__(self, other):
if self.tkeep != other.tkeep:
return False

if self.tstrb is not None and other.tstrb is not None:
if self.tstrb != other.tstrb:
return False

if self.tid is not None and other.tid is not None:
if type(self.tid) in (int, bool) and type(other.tid) is list:
for k in other.tid:
Expand Down Expand Up @@ -214,6 +230,7 @@ def __repr__(self):
return (
f"{type(self).__name__}(tdata={self.tdata!r}, "
f"tkeep={self.tkeep!r}, "
f"tstrb={self.tstrb!r}, "
f"tid={self.tid!r}, "
f"tdest={self.tdest!r}, "
f"tuser={self.tuser!r}, "
Expand All @@ -234,7 +251,7 @@ def __bytes__(self):
class AxiStreamBus(Bus):

_signals = ["tdata"]
_optional_signals = ["tvalid", "tready", "tlast", "tkeep", "tid", "tdest", "tuser"]
_optional_signals = ["tvalid", "tready", "tlast", "tkeep", "tstrb", "tid", "tdest", "tuser"]

def __init__(self, entity=None, prefix=None, **kwargs):
super().__init__(entity, prefix, self._signals, optional_signals=self._optional_signals, **kwargs)
Expand All @@ -251,7 +268,7 @@ def from_prefix(cls, entity, prefix, **kwargs):
class AxiStreamBase(Reset):

_signals = ["tdata"]
_optional_signals = ["tvalid", "tready", "tlast", "tkeep", "tid", "tdest", "tuser"]
_optional_signals = ["tvalid", "tready", "tlast", "tkeep", "tstrb", "tid", "tdest", "tuser"]

_type = "base"

Expand Down Expand Up @@ -306,6 +323,14 @@ def __init__(self, bus, clock, reset=None, reset_active_level=True,
self.byte_lanes = len(self.bus.tkeep)
if byte_size is not None or byte_lanes is not None:
raise ValueError("Cannot specify byte_size or byte_lanes if tkeep is connected")
if hasattr(self.bus, "tstrb") and (len(self.bus.tstrb) != self.byte_lanes):
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If both tkeep and tstrb are provided, then they should be the same size.

raise ValueError("tstrb must be the same width as tkeep")

elif hasattr(self.bus, "tstrb"):
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tstrb only provided, we can use that to get byte_lanes. Also reflected in the docs

self.byte_lanes = len(self.bus.tstrb)
if byte_size is not None or byte_lanes is not None:
raise ValueError("Cannot specify byte_size or byte_lanes if tstrb is connected")

else:
if byte_lanes is not None:
self.byte_lanes = byte_lanes
Expand Down Expand Up @@ -484,6 +509,8 @@ def _handle_reset(self, state):
self.bus.tlast.value = 0
if hasattr(self.bus, "tkeep"):
self.bus.tkeep.value = 0
if hasattr(self.bus, "tstrb"):
self.bus.tstrb.value = 0
if hasattr(self.bus, "tid"):
self.bus.tid.value = 0
if hasattr(self.bus, "tdest"):
Expand All @@ -505,6 +532,7 @@ async def _run(self):
has_tvalid = hasattr(self.bus, "tvalid")
has_tlast = hasattr(self.bus, "tlast")
has_tkeep = hasattr(self.bus, "tkeep")
has_tstrb = hasattr(self.bus, "tstrb")
has_tid = hasattr(self.bus, "tid")
has_tdest = hasattr(self.bus, "tdest")
has_tuser = hasattr(self.bus, "tuser")
Expand Down Expand Up @@ -536,13 +564,15 @@ async def _run(self):
tdata_val = 0
tlast_val = 0
tkeep_val = 0
tstrb_val = 0
tid_val = 0
tdest_val = 0
tuser_val = 0

for offset in range(self.byte_lanes):
tdata_val |= (frame.tdata[frame_offset] & self.byte_mask) << (offset * self.byte_size)
tkeep_val |= (frame.tkeep[frame_offset] & 1) << offset
tstrb_val |= (frame.tstrb[frame_offset] & 1) << offset
tid_val = frame.tid[frame_offset]
tdest_val = frame.tdest[frame_offset]
tuser_val = frame.tuser[frame_offset]
Expand All @@ -563,6 +593,8 @@ async def _run(self):
self.bus.tlast.value = tlast_val
if has_tkeep:
self.bus.tkeep.value = tkeep_val
if has_tstrb:
self.bus.tstrb.value = tstrb_val
if has_tid:
self.bus.tid.value = tid_val
if has_tdest:
Expand Down Expand Up @@ -673,6 +705,7 @@ async def _run(self):
has_tvalid = hasattr(self.bus, "tvalid")
has_tlast = hasattr(self.bus, "tlast")
has_tkeep = hasattr(self.bus, "tkeep")
has_tstrb = hasattr(self.bus, "tstrb")
has_tid = hasattr(self.bus, "tid")
has_tdest = hasattr(self.bus, "tdest")
has_tuser = hasattr(self.bus, "tuser")
Expand All @@ -691,16 +724,18 @@ async def _run(self):
if tready_sample and tvalid_sample:
if not frame:
if self.byte_size == 8:
frame = AxiStreamFrame(bytearray(), [], [], [], [])
frame = AxiStreamFrame(bytearray(), [], [], [], [], [])
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These caught me out, and may catch out users. the API change may justify a major version uplift

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could mitigate this by adjusting the ordering of tstrb in the constructor arguments?

else:
frame = AxiStreamFrame([], [], [], [], [])
frame = AxiStreamFrame([], [], [], [], [], [])
frame.sim_time_start = get_sim_time()
self.active = True

for offset in range(self.byte_lanes):
frame.tdata.append((self.bus.tdata.value.integer >> (offset * self.byte_size)) & self.byte_mask)
if has_tkeep:
frame.tkeep.append((self.bus.tkeep.value.integer >> offset) & 1)
if has_tstrb:
frame.tstrb.append((self.bus.tstrb.value.integer >> offset) & 1)
if has_tid:
frame.tid.append(self.bus.tid.value.integer)
if has_tdest:
Expand Down Expand Up @@ -772,6 +807,7 @@ async def _run(self):
has_tvalid = hasattr(self.bus, "tvalid")
has_tlast = hasattr(self.bus, "tlast")
has_tkeep = hasattr(self.bus, "tkeep")
has_tstrb = hasattr(self.bus, "tstrb")
has_tid = hasattr(self.bus, "tid")
has_tdest = hasattr(self.bus, "tdest")
has_tuser = hasattr(self.bus, "tuser")
Expand All @@ -792,16 +828,18 @@ async def _run(self):
if tready_sample and tvalid_sample:
if not frame:
if self.byte_size == 8:
frame = AxiStreamFrame(bytearray(), [], [], [], [])
frame = AxiStreamFrame(bytearray(), [], [], [], [], [])
else:
frame = AxiStreamFrame([], [], [], [], [])
frame = AxiStreamFrame([], [], [], [], [], [])
frame.sim_time_start = get_sim_time()
self.active = True

for offset in range(self.byte_lanes):
frame.tdata.append((self.bus.tdata.value.integer >> (offset * self.byte_size)) & self.byte_mask)
if has_tkeep:
frame.tkeep.append((self.bus.tkeep.value.integer >> offset) & 1)
if has_tstrb:
frame.tstrb.append((self.bus.tstrb.value.integer >> offset) & 1)
if has_tid:
frame.tid.append(self.bus.tid.value.integer)
if has_tdest:
Expand Down