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.

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

Fix strb of Axi4Master (spinal.lib.bus.amba4.axi.sim) #1692

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

Nik-Sch
Copy link
Contributor

@Nik-Sch Nik-Sch commented Mar 26, 2025

Context, Motivation & Description

The strb for the Axi4Master (sim) was wrong for aligned transfers with len>0. I fixed this and added a simple testbench that verifies the Axi4Master against the AxiMemorySim.

Impact on code generation

None

Checklist

  • Unit tests were added
  • API changes are or will be documented:

@Dolu1990
Copy link
Member

Hi,

(~((BigInt(1) << padBack) - 1)) & ((BigInt(1) << bytePerBeat) - 1)

Does it work for burst where size is smaller than the whole bus width ?
Seems like & ((BigInt(1) << bytePerBeat) - 1) will not work in all cases (last beat not finishing at the start of the data word.

@Nik-Sch
Copy link
Contributor Author

Nik-Sch commented Mar 28, 2025

Yes, you are correct, there were some weird bugs...
I added tests for unaligned transfers and fixed the problems. Should I test something else?

@@ -274,35 +273,40 @@ case class Axi4Master(axi: Axi4, clockDomain: ClockDomain, name: String = "unnam
}
assert(len <= 255, s"max burst in one transaction is 256")
val bytePerBeat = 1 << size
val bytes = (len + 1) * bytePerBeat
val actualLen = ((data.length / bytePerBeat.toFloat).ceil.toInt - 1) min len
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why there is actualLen ?
What is the purpose of the len argument then ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the len a larger transaction than the data.length (i.e. (len + 1) * bytePerBeat > data.length), the axi4master would issue more beats (transfer in axi notion) after the last data transfer. This would lead to zeros (at least in verilator) being written after the actual last byte.

This often happens when using the write or writeCB function where multiple transfers are initiated until the data is exhausted. I could also move the actualLen calculation to writeCB but I found it quite unexpected behavior that the master would write (a lot of) zeros (with active strb) after the data I want it to write.

Copy link
Member

Choose a reason for hiding this comment

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

My understanding so far was that the len parameter is the number of beat, not the number of bytes in the data.
See the asserts : assert(len <= 255, s"max burst in one transaction is 256")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, len specifies the number of transfers (beats in this naming here) in one transaction (off by one per axi standard). But the old version of the AXI Master would always issue a full transaction regardless of how many data bytes are provided.
An example:

  • data.length = 20 (20 bytes)
  • size = 3 -> bytePerBeat = 8
  • len = 7 -> 1 transaction is 8 transfers of 8 bytes each, resulting in 64 written bytes.

In the old implementation this would have written the 20 input bytes at the specified address and then additionally 44 bytes 0x00. I fixed this by using the len parameter rather as a maxLen and then calculating the actualLen to be 2 (3 transfers) where the strb in the last transfer is set such that only 4 of the 8 bytes are written.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants