Question about axi_stream_protocol_checker rule 5.

[bewimm]

Rule 5 of the AXI stream protocol checker ensures that tdata is valid when tvalid is asserted.
I am wondering if that should also be the case when not all bytes are valid (i.e. tkeep /= "1111").

For example the following trace results in an error:
# 62490000 ps - master:rule 5 - ERROR - Not unknown check failed for tdata when tvalid is high - Got ----_----_----_----_----_----_1111_0000.

The workaround is trivial, but I like setting unused byte lanes to '-' to easily see downstream issues.

Is the current behavior of the checker intentional or just an oversight?

[umarcor]

I'd say it's an oversight. tkeep should be taken into account.

[LarsAsplund]

The rules are created from ARM's own document "AMBA® 4 AXI4™, AXI4-Lite™, and
AXI4-Stream™ Protocol Assertions" (https://documentation-service.arm.com/static/5f106bd90daa596235e808ed?token=). Nevertheless, I agree that this looks like an oversight. I will give it some thinking to see if there are some drawbacks of making this change.

[LarsAsplund]

I think the problem here, and the reason why ARM made the rule the way they did, is that tkeep is an optional signal, i.e. the receiver of tdata containing null bytes (the bytes with - in your case) may not support tkeep and can't ignore any byte.

I see that the suggested change is a useful extension but it should be made an optional extension, allow_x_in_null_bytes

Also note that there can be null bytes if tkeep is high and tstrb is low. That scenario should also be covered by such an extension..

[bewimm]

I think the problem here, and the reason why ARM made the rule the way they did, is that tkeep is an optional signal, i.e. the receiver of tdata containing null bytes (the bytes with - in your case) may not support tkeep and can't ignore any byte.

I see that the suggested change is a useful extension but it should be made an optional extension, allow_x_in_null_bytes

Can you describe how you imagined this to work some more? I was looking for an existing component that implements an extension or something similar but couldn't find anything.
I think the changes would involve more than axi_stream_protocol_checker since the ports of that entity are fully constrained (i.e. keep and strb are present).
I'm also not entirely sure if one of the functions generating a transaction here works correctly. The specification says this about the defaults:

Also note that there can be null bytes if tkeep is high and tstrb is low. That scenario should also be covered by such an extension..

https://developer.arm.com/documentation/ihi0051/a/Interface-Signals/Byte-qualifiers/TKEEP-and-TSTRB-combinations seems to indicate that only TKEEP = '0' and TSTRB = '0' denotes a null byte.

[LarsAsplund]

I'm thinking that allow_x_in_null_bytes is a new parameter to the new_axi_stream_protocol_checker function (placed last and with default value false to be backward compatible). That configuration is stored in axi_stream_protocol_checker_t which makes it available to an instance of axi_stream_protocol_checker through the protocol_checker generic. The behaviour of rule 5 then depends on this configuration.

I think the default value for a parameter like tkeep in a push function should be "absent", i,e. the master processing that push command is not expected to have a tkeep output or have its tkeep output unconnected. If a mistake is made and that output is connected anyway it would be good if there is a way to detect that mistake. Driving all zeros means that the slave of that master won't receive any useful data and that is hopefully enough to detect the mistake

https://developer.arm.com/documentation/ihi0051/a/Interface-Signals/Byte-qualifiers/TKEEP-and-TSTRB-combinations seems to indicate that only TKEEP = '0' and TSTRB = '0' denotes a null byte.

You're right, the combination I was referring to is called a position byte. However, that is also a byte containing no information.

[bewimm]

I'm thinking that allow_x_in_null_bytes is a new parameter to the new_axi_stream_protocol_checker function (placed last and with default value false to be backward compatible). That configuration is stored in axi_stream_protocol_checker_t which makes it available to an instance of axi_stream_protocol_checker through the protocol_checker generic. The behaviour of rule 5 then depends on this configuration.

I updated PR #799, it would be great if you could take a look.

You're right, the combination I was referring to is called a position byte. However, that is also a byte containing no information.

I don't quite understand what the spec is saying for this case. My understanding was that tdata must still be valid, but the actual contents are not part of the normal stream data.

[LukasVik]

I don't quite understand what the spec is saying for this case. My understanding was that tdata must still be valid, but the actual contents are not part of the normal stream data.

My understanding is the opposite. For example https://developer.arm.com/documentation/ihi0051/a/Interface-Signals/Byte-qualifiers/TSTRB-qualification?lang=en reads

When TKEEP is asserted, TSTRB is used to indicate whether the associated byte is a data byte or a position byte. When TSTRB is asserted HIGH it indicates that the associated byte contains valid information, and is a data byte. When TSTRB is deasserted LOW it indicates that the associated byte does not contain valid information and is a position byte.

A position byte is used to indicate the correct relative position of the data bytes within the stream.

The "When TSTRB is deasserted LOW it indicates that the associated byte does not contain valid information and is a position byte." part implies to me that it is perfectly legal for an AXI-Stream master to drive a position byte with e.g. '-'.

Extending the check to allow undefined position bytes as well might make the name allow_x_in_null_bytes misleading? Perhaps allow_x_in_non_data_bytes instead? (the standard's nomenclature is summarized here: https://developer.arm.com/documentation/ihi0051/a/Interface-Signals/Byte-qualifiers/TKEEP-and-TSTRB-combinations?lang=en)

[bewimm]

My understanding is the opposite.

IMO, the naming in the spec is not very intuitive, but if I look at in the context of a resizer (32->8) for example then this makes sense to me:

tkeep := "1101";
tstrb := "1001";
tdata :=  x"01234567";

The output of the resizer can (should) be:

tvalid = [ '1',  '0',  '1',  '1']
tdata  = ["67", "XX", "XX", "01"]

So like you said, the data value for a position byte doesn't matter, but it cannot be removed.

Extending the check to allow undefined position bytes as well might make the name allow_x_in_null_bytes misleading? Perhaps allow_x_in_non_data_bytes instead?

Thanks for taking a look. I'll try to update it this week.

[LukasVik]

Agreed, but just to be clear I do believe your example still needs a tstrb:

tvalid = [ '1',  '0',  '1',  '1']
tstrb  = [ '1',  'X',  '0',  '1']
tdata  = ["67", "XX", "XX", "01"]

[Nik-Sch]

What is the status here?
I am very interested in this fix. If @bewimm doesn't continue #799, I would like to open a new PR on this...

[LukasVik]

@Nik-Sch I am also very interested in it. I have implemented workarounds around all protocol checker instances in order to solve it in my code base. But it would be nicer to have it merge here in VUnit officially.

I have reviewed #799 before and I looked through it again now, and everything looks good to me. IMHO, ready to merge.

[LarsAsplund]

Merged #799