util_axis_fifo: Fix tkeep signal value when KEEP_EN is 0

[IstvanZsSzekely]

PR Description

Changed the tkeep default value to be 0 regardless of data width then KEEP_EN is set to 0.
Wasn't tested on any of the project level testbenches or HDL projects and they shouldn't be affected either.
Tested in the DMAC IP level testbenches found in this repository, fixes a compilation issue and the test as well.

PR Type

• Bug fix (change that fixes an issue)
• New feature (change that adds new functionality)
• Breaking change (has dependencies in other repos or will cause CI to fail)

PR Checklist

• I have followed the code style guidelines
• I have performed a self-review of changes
• I have compiled all hdl projects and libraries affected by this PR
• I have tested in hardware affected projects, at least on relevant boards
• I have commented my code, at least hard-to-understand parts
• I have signed off all commits from this PR
• I have updated the documentation (wiki pages, ReadMe files, Copyright etc)
• I have not introduced new Warnings/Critical Warnings on compilation
• I have added new hdl testbenches or updated existing ones

[gastmaier]

If not using tkeep, the signal should be one.

icarus hinted correctly, DATA_WIDTH is 5 => 5/8 = 0, and therefore concatenation repeat may not be zero.
tkeep width resolves into [-1:0].

#199 localparam BEATS_PER_BURST_WIDTH_SRC = BYTES_PER_BURST_WIDTH - BYTES_PER_BEAT_WIDTH_SRC;
#424 .DATA_WIDTH(BEATS_PER_BURST_WIDTH_SRC), // i_src_dest_bl_fifo

The tb configuration is SRC AXI MM, DEST FIFO

If we can ignore this error, we can apply this patch to tie tkeep ~0:
0001-util_axis_fifo-Tie-tkeep-high-if-disable.patch
But I believe further investigation is needed.

@PIoandan do you have any insights?

note, running with xsim may be easier to investigate:

 SIMULATOR=xsim ./dma_write_shutdown_tb

[IstvanZsSzekely]

If not using tkeep, the signal should be one.

icarus hinted correctly, DATA_WIDTH is 5 => 5/8 = 0, and therefore concatenation repeat may not be zero. tkeep width resolves into [-1:0].

#199 localparam BEATS_PER_BURST_WIDTH_SRC = BYTES_PER_BURST_WIDTH - BYTES_PER_BEAT_WIDTH_SRC;
#424 .DATA_WIDTH(BEATS_PER_BURST_WIDTH_SRC), // i_src_dest_bl_fifo

The tb configuration is SRC AXI MM, DEST FIFO

If we can ignore this error, we can apply this patch to tie tkeep ~0: 0001-util_axis_fifo-Tie-tkeep-high-if-disable.patch But I believe further investigation is needed.

@PIoandan do you have any insights?

note, running with xsim may be easier to investigate:

 SIMULATOR=xsim ./dma_write_shutdown_tb

The reason behind hardcoding the tkeep value to 0 in the code is that if the TKEEP_EN is set to 0, then tkeep on the master and slave does not matter and all data is considered valid on the output (m_axis).

Case: If the tkeep signals do matter, but the designer accidentally leaves TKEEP_EN on 0. While tkeep is set to 1 (all data is assumed to be correct and valid), this may lead to hard to find bugs in the IP development, if not all tkeep bit are one on the input (s_axis). With tkeep bits set to 0, this will quickly lead to the IP in development to not function correctly, as it invalidates all data from the start.

I agree with the fact that the tkeep signals should be set to 1 when TKEEP_EN is 0, however, from my point of view, this can lead to more headaches down the road, when developing new IPs that rely on the FIFO module.

PS: I don't trust myself with finding such a bug without a good amount of headache.

[IstvanZsSzekely]

Upon further inspection of the thrown error for DATA_WIDTH being less than 1, one potential fix for this issue is to use variable size checking at compilation/synthesis time, something similar like here. It will ensure that the parameters of the IP are within reasonable values (non-negative in this case), and catch invalid configurations. This will not guarantee that the IP will work however.

[IstvanZsSzekely]

Upon further inspection of the thrown error for DATA_WIDTH being less than 1, one potential fix for this issue is to use variable size checking at compilation/synthesis time, something similar like here. It will ensure that the parameters of the IP are within reasonable values (non-negative in this case), and catch invalid configurations. This will not guarantee that the IP will work however.

Elaboration/compilation time parameter checking is unavailable in Verilog, this is a SystemVerilog system function call. The only option that seems to be viable is described here.

[gastmaier]

I would just ~0 on these conditions that tkeep is unused.
The reasoning is that this will be optimized away since is unconnected when unused anyway.
My concern is at IP level, when TKEEP is unused but needs to be connected to other IP that requires TKEEP, and yielding 0 would be wrong.

[IstvanZsSzekely]

I checked the AXI-stream standard (which I forgot about until this point) and set both keep and last signals to 1, as this is what the standard requires

[gastmaier]

https://documentation-service.arm.com/static/60d5e2510320e92fa40b4788?token=

3.1.3 Optional TLAST

1. Set TLAST LOW. This indicates that all transfers are within the same packet. This option
   provides maximum opportunity for merging and upsizing but means that transfers could
   be delayed in a stream with intermittent bursts.
2. Set TLAST HIGH. This indicates that all transfers are individual packets. This option
   ensures that transfers do not get delayed in the infrastructure.